-
Notifications
You must be signed in to change notification settings - Fork 26
INTPYTHON-527 Add Queryable Encryption support #329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
django_mongodb_backend/management/commands/get_encrypted_fields_map.py
Outdated
Show resolved
Hide resolved
django_mongodb_backend/management/commands/get_encrypted_fields_map.py
Outdated
Show resolved
Hide resolved
django_mongodb_backend/management/commands/get_encrypted_fields_map.py
Outdated
Show resolved
Hide resolved
django_mongodb_backend/management/commands/get_encrypted_fields_map.py
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Any progress on this? Do we need to ask @blink1073 for help? |
@aclark4life Soooo ... I don't know what you've done around that area so far, but it's great that you're bringing it up, I didn't realize we'd have to still worry about this part because I don't think the PR currently concerns itself at all with how we integrate with crypt_shared/mongocryptd? So I'll explain the whole story here, knowing that I'm risking telling you things you're alread well aware of 🙂
Operationally, So we generally recommend using the crypt_shared library, which can be achieved by passing Given that this is a new feature here, I'd strongly consider setting As far as the tests here are concerned themselves, I imagine they're passing locally for you because you do have |
Can we document our way around this by recommending Also what about bundling that library in the |
Maybe, but I think we'd want to have a conversation around what the typical expectations here are. You'll generally want to have development and production environments behave similarly, and you'd still need to have a plan for what to do when mongocryptd does get deprecated eventually (long-term, I think it's fair to expect this to happen). You'll also still be in a position where you need to download and install mongocryptd, the only case in which this requirement goes away is the one where you happen to have the enterprise MongoDB server binaries already ready in your
This question comes up on a regular basis 🙂 Here's a Slack thread from April, which was one of the last times we spoke about this. tl;dr: Yes, the setup process for CSFLE/QE is involved, and we'd like to make it easier. Currently, there is a requirement for the user to explicitly acknowledge that they have read and accepted the enterprise license agreement and that they are an Atlas or EA customer. Bundling this library with regular packages that can be installed via a regular package manager command like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good! I've provided comments and feedback around some documentation and clarifications, but will approve once those are addressed.
ae765eb
to
886b1e0
Compare
5b2bc07
to
bddae41
Compare
django_mongodb_backend/management/commands/showencryptedfieldsmap.py
Outdated
Show resolved
Hide resolved
django_mongodb_backend/management/commands/showencryptedfieldsmap.py
Outdated
Show resolved
Hide resolved
raise ImproperlyConfigured( | ||
"Encrypted fields found but " | ||
"DATABASES[[self.connection.alias}]['OPTIONS'] is missing " | ||
"auto_encryption_opts. Please set `auto_encryption_opts` " | ||
"in the connection settings." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you try adding a test for this exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested it manually but haven't written a test for it yet.
"KMS_PROVIDERS": {}, | ||
"KMS_CREDENTIALS": {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For KMS_CREDENTIALS, I believe the options are documented here: https://pymongo.readthedocs.io/en/stable/api/pymongo/encryption.html#pymongo.encryption.ClientEncryption.create_data_key (but pymongo docs are deprecated?)
So a minimal example for AWS: "KMS_CREDENTIALS": {"aws": {"region": "...", "key": "..."}
- Fix rebase merge conflict edits - Remove integer field FIXME comments - Remove pos_int
Don't break the build!
Don't break the build Part II
0e7b955
to
6afce5a
Compare
|
||
.. _server-side-queryable-encryption-settings: | ||
|
||
Server-side Queryable Encryption |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely not a phrase we should be using in documentation; the core point of CSFLE/QE is that the encryption itself is primarily a client-side feature, and this title sounds like some of the encryption logic actually happens on the server side. This should be something like "Server-side schema for Queryable Encryption".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😂 OK will fix
|
||
.. _client-side-queryable-encryption: | ||
|
||
Client-side Queryable Encryption |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
|
||
Additionally, you will need to ensure that the :ref:`crypt shared library is | ||
available <manual:qe-reference-shared-library>` to your Python environment. | ||
The crypt shared library is recommended over libmongocrypt for Queryable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhh ... what? Is this supposed to be
The crypt shared library is recommended over libmongocrypt for Queryable | |
The crypt shared library is recommended over mongocryptd for Queryable |
?
… | ||
"OPTIONS": { | ||
"auto_encryption_opts": AutoEncryptionOpts( | ||
… |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this include an example of how to specify the crypt_shared
library path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Will add.
c7f360e
to
31b24f6
Compare
Previous attempts and additional context here: